Skip to content

ruff rules: TCH → TC #3032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
May 25, 2025
Merged

ruff rules: TCH → TC #3032

merged 4 commits into from
May 25, 2025

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented May 4, 2025 •
edited
Loading

Ruff v0.8.0 changes the error codes for the rules in the flake8-type-checking category from TCH to TC.

Not sure about applying TC006:

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/user-guide/*.rst
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label May 4, 2025
@dstansby dstansby mentioned this pull request May 14, 2025
Merged
@DimitriPapadopoulos
The ruff ruleset is TC, not TCH
d5b59c5
@DimitriPapadopoulos
Apply ruff/flake8-type-checking rule TC006
3143e97 
TC006 Add quotes to type expression in `typing.cast()`
@DimitriPapadopoulos
Apply ruff/flake8-type-checking rule TC003
2af5bcf 
TC003 Move standard library import into a type-checking block
@dstansby
Copy link
Contributor

I'll let other developers chime in, but personally I'm 👎 to putting the typing expressions all in strings.

@d-v-b
Copy link
Contributor

d-v-b commented May 25, 2025

I found this argument compelling. It seems that casting to union types can have runtime overhead. I think we should err on the safe side when it comes to performance, which suggests quoting the types.

@dstansby
Copy link
Contributor

dstansby commented May 25, 2025 •
edited
Loading

I just found a reason to do this myself (6dd60e2), so actually 👍 for this. Thanks for digging up info about performance too, I agree that's also a good reason.

@dstansby dstansby enabled auto-merge (squash) May 25, 2025 21:15
@dstansby dstansby merged commit 0dd797f into zarr-developers:main May 25, 2025
29 of 30 checks passed
@DimitriPapadopoulos DimitriPapadopoulos deleted the TC branch May 26, 2025 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
needs release notes Automatically applied to PRs which haven't added release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DimitriPapadopoulos @dstansby @d-v-b